Read package manifests with utf-8-sig so a BOM does not drop the package - #5321
Open
arpitjain099 wants to merge 2 commits into
Open
arpitjain099 wants to merge 2 commits into
arpitjain099 wants to merge 2 commits into
Conversation
Python's utf-8 codec does not consume a byte order mark. It decodes the three bytes to U+FEFF, and json.load then raises "Unexpected UTF-8 BOM (decode using utf-8-sig)". packagedcode opens every manifest with encoding='utf-8', and recognize.py _parse() swallows the exception and continues, so a manifest that carries a BOM disappears from the scan with scan_errors left empty. The input is valid. npm, Composer and the other package managers read these files, and a BOM is what several Windows editors and PowerShell's Out-File write by default. utf-8-sig differs from utf-8 only in consuming a leading BOM if one is there, so this is a safe swap at all 25 manifest read sites in packagedcode. There was no BOM-aware decode anywhere in the tree before this. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
package.jsonwith a UTF-8 BOM makes the whole package vanish from the scan, andscan_errorsstays empty, so nothing in the output says anything went wrong.Same file twice, one with the three BOM bytes in front, scanned with the released
scancode-toolkit 32.5.0:The input is not malformed.
node -e "require('./package.json')"reads the BOM copy fine, and a BOM is what PowerShell'sOut-Fileand several Windows editors write by default, so this shows up on real repositories rather than only in constructed cases.The cause is the codec. Python's
utf-8does not consume a BOM, it decodes those bytes to U+FEFF, andjson.loadthen raises with a message that names the fix:recognize.py:155-162catches that andcontinues without recording anything, which is why the failure is silent. Its sibling one layer up atplugin_package.py:481does append the traceback toresource.scan_errors, so assembly failures are visible and parse failures are not.I fixed the decode rather than the swallow, because the file is valid input and the package should be detected, not reported as an error.
utf-8-sigis identical toutf-8except that it consumes a leading BOM if one is present, so the swap is safe wherever a manifest is read, andgrep -rn "utf-8-sig\|ufeff\|strip_bom" --include="*.py" src/returned nothing before this, meaning there was no BOM handling anywhere to be consistent with. That is all 25encoding='utf-8'sites insrc/packagedcode/, across 15 handlers.The swallow in
recognize.pyis worth fixing too, but it is a separate change with a wider blast radius, so I left it alone here.Verification
tests/packagedcode/test_utf8_bom.pyparametrizes four handlers that between them cover the JSON manifest shape (npm, bower, Composer, haxelib), once with a BOM and once without. The four BOM cases fail ondevelopwith theJSONDecodeErrorabove and pass with the change; the four no-BOM cases pass either way and are there so a regression in the plain path cannot hide.For regressions I ran the tests for every handler I touched, before and after, against
developat384b62a9:test_npm94,test_rpm76,test_conda20,test_readme17,test_conan12,test_godeps11,test_swift11,test_phpcomposer10,test_freebsd9,test_bower6,test_haxe5,test_about4,test_cran2 all passed, andtest_chefwas 4 failed / 8 passed in both runs, so those four are pre-existing and unrelated.